Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skip failing api calls #7059

Merged

Conversation

andrewjstone
Copy link
Contributor

No description provided.

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look at this! Looks like this PR hit #4949

We talked about not bailing on failures on last week's huddle, but I'm unsure if there was a consensus on which services we want to warn about only? ClickHouse db initialization seems like a good candidate for not bailing out due to a failure, so I'm all in for this change. Just want to make sure we have some sort of follow up issue or something about which errors we want to skip and which ones should stop the process entirely.

if let Err(e) = client.init_db().await {
warn!(
log,
"failed to initialize single-node clickhouse database: {e}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to want to do the same for keepers and replicated servers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! I'm going to do this very shortly.

@andrewjstone
Copy link
Contributor Author

Thanks for taking a look at this! Looks like this PR hit #4949

We talked about not bailing on failures on last week's huddle, but I'm unsure if there was a consensus on which services we want to warn about only? ClickHouse db initialization seems like a good candidate for not bailing out due to a failure, so I'm all in for this change. Just want to make sure we have some sort of follow up issue or something about which errors we want to skip and which ones should stop the process entirely.

@davepacheco opened #6999 last week to cover this. It doesn't have any specific service call outs yet though.

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davepacheco opened #6999 last week to cover this. It doesn't have any specific service call outs yet though.

Ha! Looks like I missed that issue somehow. Looks good to me then!

Copy link
Contributor

@plotnick plotnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for looking at this and coming up with such a simple fix!

@andrewjstone
Copy link
Contributor Author

Thank you very much for looking at this and coming up with such a simple fix!

You are very welcome. Feel free to merge this into your branch or copy or whatever you desire.

@plotnick plotnick merged commit 97d4c2c into single-node-clickhouse-init Nov 14, 2024
16 checks passed
@plotnick plotnick deleted the single-node-clickhouse-test-fixes branch November 14, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants